Extract inbound tcp and websocket connection flow to separate classes, unify retrying #773

Vlatombe · 2024-10-25T09:43:42Z

Extracts Inbound TCP and websocket connection flows to separate classes, and unify the retrying logic, so that both Inbound TCP and Websocket use retries, with exponential retries.

Bumped requirement to Java 17, since we are past End-october, and records makes things pretttier.

Testing done

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

A more ambitious attempt to extract logic from Engine into separate classes

Reorganize code, javadoc

jglick · 2024-11-04T17:51:39Z

incremental diff

src/main/java/hudson/remoting/Engine.java

src/main/java/org/jenkinsci/remoting/engine/AbstractEndpointConnector.java

src/main/java/org/jenkinsci/remoting/engine/EndpointConnector.java

jglick · 2024-11-04T17:58:22Z

src/main/java/org/jenkinsci/remoting/engine/InboundTCPConnector.java

+        private final RSAPublicKey publicKey;
+        private final Map<String, String> headers;
+
+        public EngineJnlpConnectionStateListener(RSAPublicKey publicKey, Map<String, String> headers) {
+            this.publicKey = publicKey;
+            this.headers = headers;
+        }


Could be a record. (see #749)

Too bad JnlpConnectionStateListener is an abstract class (for no good reason)

src/main/java/org/jenkinsci/remoting/engine/RetryUtils.java

jglick · 2024-11-04T18:03:27Z

Seems OK from a quick glance, though the diff is too long to readily see what is actually being edited.

And implement records

Vlatombe · 2024-11-05T09:03:18Z

src/test/java/hudson/remoting/EngineTest.java

@@ -146,7 +146,7 @@ public void error(Throwable t) {

    private static class NoReconnectException extends RuntimeException {}

-    @Test(timeout = 30_000)
+    @Test(timeout = 10_000)


Shortening timeout since the new retry logic is snappier

Vlatombe · 2024-11-05T09:08:53Z

src/main/java/org/jenkinsci/remoting/engine/InboundTCPConnector.java

+        private final RSAPublicKey publicKey;
+        private final Map<String, String> headers;
+
+        public EngineJnlpConnectionStateListener(RSAPublicKey publicKey, Map<String, String> headers) {
+            this.publicKey = publicKey;
+            this.headers = headers;
+        }


Too bad JnlpConnectionStateListener is an abstract class (for no good reason)

Vlatombe · 2024-11-05T09:09:20Z

src/main/java/hudson/remoting/Engine.java

@@ -269,7 +222,7 @@ public Engine(EngineListener listener, List<URL> hudsonUrls, String secretKey, S

    public Engine(
            EngineListener listener,
-            List<URL> hudsonUrls,


well... long overdue.

Vlatombe · 2024-11-05T09:10:09Z

src/main/java/hudson/remoting/Engine.java

-                SSLContext context;
-                // prepare our SSLContext
-                try {
-                    context = SSLContext.getInstance("TLS");
-                } catch (NoSuchAlgorithmException e) {
-                    throw new IllegalStateException("Java runtime specification requires support for TLS algorithm", e);
-                }
-                char[] password = "password".toCharArray();
-                KeyStore store;
-                try {
-                    store = KeyStore.getInstance(KeyStore.getDefaultType());
-                } catch (KeyStoreException e) {
-                    throw new IllegalStateException("Java runtime specification requires support for JKS key store", e);
-                }
-                try {
-                    store.load(null, password);
-                } catch (NoSuchAlgorithmException e) {
-                    throw new IllegalStateException("Java runtime specification requires support for JKS key store", e);
-                } catch (CertificateException e) {
-                    throw new IllegalStateException("Empty keystore", e);
-                }
-                KeyManagerFactory kmf;
-                try {
-                    kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
-                } catch (NoSuchAlgorithmException e) {
-                    throw new IllegalStateException(
-                            "Java runtime specification requires support for default key manager", e);
-                }
-                try {
-                    kmf.init(store, password);
-                } catch (KeyStoreException | NoSuchAlgorithmException | UnrecoverableKeyException e) {
-                    throw new IllegalStateException(e);
-                }
-                try {
-                    context.init(kmf.getKeyManagers(), new TrustManager[] {agentTrustManager}, null);
-                } catch (KeyManagementException e) {
-                    events.error(e);
-                    return;
-                }


Moved to SSLUtils

Vlatombe · 2024-11-05T09:10:33Z

src/main/java/hudson/remoting/Engine.java

    @SuppressFBWarnings(
-            value = {"REC_CATCH_EXCEPTION"},
-            justification = "checked exceptions were a mistake to begin with; connecting to Jenkins from agent")
-    private void runWebSocket() {


Moved to WebSocketConnector

Vlatombe · 2024-11-05T09:11:11Z

src/main/java/hudson/remoting/Engine.java

-    private static class ExponentialRetry {
-        final int factor;
-        final Instant beginning;
-        final Duration delay;
-        final Duration timeout;
-        final Duration incrementDelay;
-        final Duration maxDelay;
-
-        ExponentialRetry(Duration timeout) {
-            this(Duration.ofSeconds(0), timeout, 2, Duration.ofSeconds(1), Duration.ofSeconds(10));
-        }
-
-        ExponentialRetry(
-                Duration initialDelay, Duration timeout, int factor, Duration incrementDelay, Duration maxDelay) {
-            this.beginning = Instant.now();
-            this.delay = initialDelay;
-            this.timeout = timeout;
-            this.factor = factor;
-            this.incrementDelay = incrementDelay;
-            this.maxDelay = maxDelay;
-        }
-
-        ExponentialRetry(ExponentialRetry previous) {
-            beginning = previous.beginning;
-            factor = previous.factor;
-            timeout = previous.timeout;
-            incrementDelay = previous.incrementDelay;
-            maxDelay = previous.maxDelay;
-            delay = min(maxDelay, previous.delay.multipliedBy(previous.factor).plus(incrementDelay));
-        }
-
-        private static Duration min(Duration a, Duration b) {
-            return a.compareTo(b) < 0 ? a : b;
-        }
-
-        boolean timeoutExceeded() {
-            return Util.shouldBailOut(beginning, timeout);
-        }
-
-        ExponentialRetry next(EngineListenerSplitter events) throws InterruptedException {
-            var next = new ExponentialRetry(this);
-            if (next.timeoutExceeded()) {
-                events.status("Bailing out after " + DurationFormatter.format(next.timeout));
-                return null;
-            } else {
-                events.status("Waiting " + DurationFormatter.format(next.delay) + " before retry");
-                Thread.sleep(next.delay.toMillis());
-            }
-            return next;
-        }
-    }


Moved to RetryUtils

Vlatombe · 2024-11-05T09:11:53Z

src/main/java/hudson/remoting/Engine.java

-        }
-    }
-
-    private void innerRun(IOHub hub, SSLContext context, ExecutorService service) {


Moved to InboundTCPConnector

Vlatombe · 2024-11-05T09:12:14Z

src/main/java/hudson/remoting/Engine.java

@@ -1156,139 +585,13 @@ public static Engine current() {

    private static final Logger LOGGER = Logger.getLogger(Engine.class.getName());

-    @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "File path is loaded from system properties.")
-    static KeyStore getCacertsKeyStore()


Moved to SSLUtils

Vlatombe · 2024-11-05T09:12:30Z

src/main/java/hudson/remoting/Engine.java

@@ -1312,53 +615,4 @@ public String getAgentName() {
    public String getProtocolName() {
        return this.protocolName;
    }
-
-    private class EngineJnlpConnectionStateListener extends JnlpConnectionStateListener {


Moved to InboundTCPConnector

Vlatombe · 2024-11-05T09:21:20Z

src/main/java/hudson/remoting/Engine.java

-            }
+            value = {"HARD_CODE_PASSWORD", "REC_CATCH_EXCEPTION"},
+            justification = "Password doesn't need to be protected.; We need to catch all exceptions")
+    public void run() {


This method contains the key structural changes.

Vlatombe · 2024-11-05T09:57:34Z

src/main/java/org/jenkinsci/remoting/engine/InboundTCPConnector.java

+        var clientProtocols = new JnlpProtocolHandlerFactory(data.executor())
+                .withIOHub(hub)
+                .withSSLContext(context)
+                .withPreferNonBlockingIO(false) // we only have one connection, prefer blocking I/O
+                .handlers();
+        var negotiatedProtocols = clientProtocols.stream()
+                .filter(JnlpProtocolHandler::isEnabled)
+                .filter(p -> endpoint.isProtocolSupported(p.getName()))
+                .collect(Collectors.toSet());
+        var serverProtocols = endpoint.getProtocols() == null ? "?" : String.join(",", endpoint.getProtocols());
+        LOGGER.info(buildDebugProtocolsMessage(serverProtocols, clientProtocols, negotiatedProtocols));


Gives clearer output to users on which protocols are supported on either side of the connection.

jglick

OK AFAICT

src/main/java/org/jenkinsci/remoting/engine/RetryUtils.java

pom.xml

basil · 2024-11-06T18:11:49Z

See this document for a discussion of various retry strategies. Exponential backoff does appear to be a strict improvement over the status quo, as it balances retries against latency when the controller is unavailable for an unknown duration; however, in contrast with jitter-based solutions, it does not resolve contention for the controller. As in #676, testing of various retry strategies in real-world scenarios is needed to evaluate the effectiveness of those strategies. I would be fine with this PR if it was a simple refactoring that did not introduce exponential backoff. I would also be fine with introducing exponential backoff it was confirmed to have a positive impact in a functional test scenario.

Extract EndpointConnectors with 2 implementions

fb1338a

A more ambitious attempt to extract logic from Engine into separate classes

Vlatombe requested a review from jglick October 25, 2024 09:43

Vlatombe added 3 commits October 25, 2024 14:20

Spotless

a255435

Simplify

9f25128

Extract JNLPEndpointResolver out of InboundTCPConnector

b4c9850

Reorganize code, javadoc

jglick reviewed Nov 4, 2024

View reviewed changes

src/main/java/hudson/remoting/Engine.java Outdated Show resolved Hide resolved

jglick reviewed Nov 4, 2024

View reviewed changes

src/main/java/org/jenkinsci/remoting/engine/AbstractEndpointConnector.java Outdated Show resolved Hide resolved

jglick reviewed Nov 4, 2024

View reviewed changes

src/main/java/org/jenkinsci/remoting/engine/EndpointConnector.java Outdated Show resolved Hide resolved

jglick reviewed Nov 4, 2024

View reviewed changes

src/main/java/org/jenkinsci/remoting/engine/RetryUtils.java Outdated Show resolved Hide resolved

Vlatombe added 3 commits November 5, 2024 09:26

Merge branch 'master' into endpoint-connector

02f8a57

Move to JDK17

6a1ce9d

And implement records

Add restrictions and @SInCE as needed.

70f312a

Vlatombe commented Nov 5, 2024

View reviewed changes

Vlatombe added 4 commits November 5, 2024 10:23

Javadoc

1ecdd9e

Simplify noReconnectAfter handling

9ba77f7

Attempt to give slighly more memory to tests

7bc0d04

Spotbugs

55ef806

Vlatombe marked this pull request as ready for review November 5, 2024 09:56

Vlatombe commented Nov 5, 2024

View reviewed changes

Vlatombe added 4 commits November 5, 2024 11:02

Fix delay

887773e

Eat stacktrace here as it is too noisy

21150c6

Extract next delay logic to a method

9b1ab2a

Simplify loop

0d7cb97

jglick approved these changes Nov 5, 2024

View reviewed changes

timja approved these changes Nov 5, 2024

View reviewed changes

src/main/java/org/jenkinsci/remoting/engine/RetryUtils.java Outdated Show resolved Hide resolved

RetryUtils is not public

23276a1

timja reviewed Nov 6, 2024

View reviewed changes

pom.xml Show resolved Hide resolved

Update pom.xml

c3a646c

timja reviewed Nov 6, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

timja added 2 commits November 6, 2024 08:58

Update pom.xml

325f332

Update jvm.config

04af69d

timja added the enhancement For changelog: An enhancement providing new capability. label Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract inbound tcp and websocket connection flow to separate classes, unify retrying #773

Extract inbound tcp and websocket connection flow to separate classes, unify retrying #773

Vlatombe commented Oct 25, 2024 •

edited

Loading

jglick commented Nov 4, 2024

jglick Nov 4, 2024 •

edited

Loading

Vlatombe Nov 5, 2024

jglick commented Nov 4, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

Vlatombe Nov 5, 2024

jglick left a comment

basil commented Nov 6, 2024 •

edited

Loading

Extract inbound tcp and websocket connection flow to separate classes, unify retrying #773

Are you sure you want to change the base?

Extract inbound tcp and websocket connection flow to separate classes, unify retrying #773

Conversation

Vlatombe commented Oct 25, 2024 • edited Loading

Testing done

Submitter checklist

jglick commented Nov 4, 2024

jglick Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

basil commented Nov 6, 2024 • edited Loading

Vlatombe commented Oct 25, 2024 •

edited

Loading

jglick Nov 4, 2024 •

edited

Loading

basil commented Nov 6, 2024 •

edited

Loading